feat: Add Azure Naming Service #219
Conversation
tbarlow12
force-pushed
the
tabarlow/naming-service
branch
2 times, most recently
from
259a5f3 to
1e4298b
Compare
tbarlow12
force-pushed
the
tabarlow/naming-service
branch
from
1e4298b to
a529b71
Compare
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Let's discuss. There are a few things in here that don't feel quite right.
| } | ||
|
|
||
| public getParameters(config: ServerlessAzureConfig) { | ||
| public getParameters(config: ServerlessAzureConfig, namer: (resource: ArmResourceType) => string) { |
There was a problem hiding this comment.
It doesn't feel right that the getParameters(...) function now requires the function to name the resource. Since you have already added ArmResourceType enum - what if we have the ARM resource classes just take a dependency on the naming service? And call the naming service as needed to generate the name?
| @@ -1,11 +1,23 @@ | |||
| import { ServerlessAzureConfig } from "./serverless"; | |||
|
|
|||
| export enum ArmResourceType { | |||
There was a problem hiding this comment.
Do we need to expose this? Each ARM resource is already a class. Can't we use the type of class as the identifier vs adding another property to get the ARM resource type?
| * Get name of Azure resource | ||
| * @param resource ARM Resource to name | ||
| */ | ||
| public getResourceName(resource: ArmResourceType): string { |
There was a problem hiding this comment.
I'm also uneasy about this change. Currently the "naming convention" is controlled by the owning resource and the business logic / naming convention is close to the resource that solves other concerns about that resource. Moving the resource specific implementation out of the resource into another service with an every growing number of functions seems off to me. Let's discuss the overall direction here.
ArmResourceTypeenumSo far, just moved naming tests to
azureNamingService.test.ts, did not add any new tests. Will add more if needed. This is part of an effort to resolve [AB#597], regarding deployment names being too long. With this in place, it will be easier to maintain restrictions for naming conventions of each Azure Resource.